Skip to content

[SPARK-10300] [BUILD] [TESTS] Add support for test tags in run-tests.py.#8775

Closed
vanzin wants to merge 3 commits into
apache:masterfrom
vanzin:SPARK-10300
Closed

[SPARK-10300] [BUILD] [TESTS] Add support for test tags in run-tests.py.#8775
vanzin wants to merge 3 commits into
apache:masterfrom
vanzin:SPARK-10300

Conversation

@vanzin

@vanzin vanzin commented Sep 15, 2015

Copy link
Copy Markdown
Contributor

No description provided.

Marcelo Vanzin added 2 commits September 15, 2015 13:05
A new module ("tags/") was added where all test tags should be placed.
This is required because the JUnit runners for both maven and sbt require
the classes to be visible when you disable them, even if no tests actually
reference them.

To avoid a cyclic dependency, the tags dependency needs to be manually added
to all poms, which is annoying. An alternative would be to make the tag
module's pom not have spark-parent as its parent, but that would mean
replicating some information from the parent pom and fixing other code
(such as the "change-scala-version.sh" script).
@vanzin

vanzin commented Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

Re-posting previous PR with a fix for the issue that caused tests to fail in other PRs. For the changes since the last PR, see only the second commit.

I'm not super happy with needing the new module, but I couldn't find an alternative that still allowed us to tag JUnit tests. The extra noise to add the dependencies to all other poms also irks me a little bit.

/cc @pwendell to see if you have a better suggestion...

@SparkQA

SparkQA commented Sep 15, 2015

Copy link
Copy Markdown

Test build #42511 has finished for PR 8775 at commit 9e0b3c4.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Sep 16, 2015

Copy link
Copy Markdown

Test build #42514 has finished for PR 8775 at commit f3bb7b4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin

vanzin commented Sep 22, 2015

Copy link
Copy Markdown
Contributor Author

retest this please

@vanzin

vanzin commented Sep 22, 2015

Copy link
Copy Markdown
Contributor Author

any comments on the new approach?

@SparkQA

SparkQA commented Sep 22, 2015

Copy link
Copy Markdown

Test build #42844 has finished for PR 8775 at commit f3bb7b4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin

vanzin commented Sep 28, 2015

Copy link
Copy Markdown
Contributor Author

retest this please

@SparkQA

SparkQA commented Sep 28, 2015

Copy link
Copy Markdown

Test build #43062 has finished for PR 8775 at commit f3bb7b4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin

vanzin commented Oct 6, 2015

Copy link
Copy Markdown
Contributor Author

I'll run the tests once again and push this, since I haven't seen any feedback on top of the previous PR. retest this please

@mengxr

mengxr commented Oct 6, 2015

Copy link
Copy Markdown
Contributor

LGTM. Merged into master. Thanks! There might be a small issue with the default value. I will test it locally and send follow-up PR if necessary.

@SparkQA

SparkQA commented Oct 6, 2015

Copy link
Copy Markdown

Test build #43279 has finished for PR 8775 at commit f3bb7b4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen

Copy link
Copy Markdown
Contributor

@mengxr, looks like the merge commit didn't get pushed (or ASF -> Git mirroring is lagging)?

@vanzin

vanzin commented Oct 7, 2015

Copy link
Copy Markdown
Contributor Author

Yes, this was not pushed. I'll push this later if it's still around.

@vanzin

vanzin commented Oct 7, 2015

Copy link
Copy Markdown
Contributor Author

ok merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants